Add user defined metadata #5915

criccomini · 2024-06-18T20:09:27Z

Which issue does this PR close?

Rationale for this change

We would like to be able to read and write user-defined metadata for AWS, GCP, and Azure. This use-case is mentioned briefly in #4754 before the conversation shifts to tags. It appears that tags were implemented, but user-defined metadata has not yet been.

What changes are included in this PR?

Add user-defined metadata for AWS, GCP, and Azure clients.

Are there any user-facing changes?

I've added a Metadata(String) attribute to the Attribute enum. Other than that I don't believe there are any user-facing changes.

I do not believe there are any breaking changes. The user-facing change (Attributes::Metadata(String)) is additive and should be compatible. The one caveat here is I'm not sure how Rust handles enums (open vs. closed). If it handles them the way Java does (i.e. adding new enums can break old code) then this would be a breaking change. I'll leave it to the Rust experts to clarify this.

criccomini · 2024-06-18T20:14:39Z

There are a few implementation details I'd like to call out:

The reqwest header name handling returns all custom headers as lower case. So if a user sets a Metadata("FOO"), the results returned in GetResult will have Metadata("foo"). This is somewhat unintuitive.
I elected to make USER_DEFINED_METADATA_HEADER_PREFIX a const rather than static because it's referenced in certain const spots. Rust didn't like static variables referenced from const.
I'm not thrilled about the get.rs code I added, but I couldn't figure out a better way. We need to iterate over all headers with the user-defined prefix (cloud-specific) and add them to attributes. I'm open to alternative approaches.
I did not add WebDav (http) support. The client simply ignores Metadata attributes on put() right now.

Xuanwo

This PR looks good to me overall!

Some missing works could be tracking in follow up:

Add custom metadata support in multipart upload
Add custom metadata integration test
Track the current progress of webdav(http)
Local fs might support custom metadata by xattr

Some design decision like naming need reviews from @tustvold and @alamb.

Xuanwo · 2024-06-19T01:49:35Z

object_store/src/aws/client.rs

@@ -61,6 +61,7 @@ use std::sync::Arc;

 const VERSION_HEADER: &str = "x-amz-version-id";
 const SHA256_CHECKSUM: &str = "x-amz-checksum-sha256";
+const USER_DEFINED_METADATA_HEADER_PREFIX: &str = "x-amz-meta-";


nit: maybe METADATA_HEADER_PREFIX is clear enough?

Happy to rename to whatever. I chose to be quite verbose because I feel there's a lot of confusion about "metadata". You can see this play out in #4754, where the conversation muddles user-defined metadata and tags. Add in standard (non-user-defined) metadata and it gets even more murky.

Y'all let me know what to rename to, though. :)

object_store/src/client/get.rs

Xuanwo · 2024-06-19T02:02:46Z

object_store/src/http/client.rs

@@ -183,6 +183,8 @@ impl Client {
                        has_content_type = true;
                        builder.header(CONTENT_TYPE, v.as_ref())
                    }
+                    // Ignore metadata attributes
+                    Attribute::Metadata(_) => builder,


WebDAV handles custom metadata in prop, which differs from other object storage services. We can create an issue to track this.

In the meantime I think we should return an error as per https://docs.rs/object_store/latest/object_store/struct.PutOptions.html#structfield.attributes

Xuanwo · 2024-06-19T02:03:24Z

object_store/src/aws/client.rs

@@ -642,6 +647,7 @@ impl GetClient for S3Client {
        etag_required: false,
        last_modified_required: false,
        version_header: Some(VERSION_HEADER),
+        user_defined_metadata_prefix: Some(USER_DEFINED_METADATA_HEADER_PREFIX),


nit: how about using metadata_header_prefix?

object_store/src/aws/client.rs

Xuanwo · 2024-06-19T02:14:22Z

The one caveat here is I'm not sure how Rust handles enums (open vs. closed).

Rust handles this by non_exhaustive attribute.

criccomini · 2024-06-19T17:52:37Z

Thanks @Xuanwo. I've updated the PR with your suggestions. I haven't renamed the variables yet since it sounds like @alamb or @tustvold need to weigh in.

Regarding your note about "Add custom metadata integration test", I was hoping this change would test the new functionality. Is it not enough?

Regarding multipart upload, I think this is already handled. AWS's client has:

    pub async fn create_multipart(
        &self,
        location: &Path,
        opts: PutMultipartOpts,
    ) -> Result<MultipartId> {
        let response = self
            .request(Method::POST, location)
            .query(&[("uploads", "")])
            .with_encryption_headers()
            .with_attributes(opts.attributes)
            .with_tags(opts.tags)
            .idempotent(true)
            .send()
            .await?
            .bytes()
            .await
            .context(CreateMultipartResponseBodySnafu)?;

        let response: InitiateMultipartUploadResult =
            quick_xml::de::from_reader(response.reader()).context(InvalidMultipartResponseSnafu)?;

        Ok(response.upload_id)
    }

The opts.attributes in with_attributes is an Attributes, which now can contain Metadata(String). And with_attributes has been modified to set x-amz-meta- headers.

alamb

THank you @criccomini and @Xuanwo for the review

Please ping me if you need another review. I think @tustvold may be back next week and he might get to reviewing this first

criccomini · 2024-06-19T22:56:08Z

Sounds good! I'll check in next week. 😄

tustvold · 2024-06-23T09:58:27Z

object_store/src/attributes.rs

+    /// Specifies a user-defined metadata field for the object
+    ///
+    /// The String is a user-defined key
+    Metadata(String),


It's likely a silliness that will make no measurable performance difference, but given these will often be static strings, I wonder what you think of making this Cow<'static, str>

tustvold · 2024-06-23T09:59:26Z

object_store/src/client/get.rs

+    if let Some(prefix) = T::HEADER_CONFIG.user_defined_metadata_prefix {
+        for (key, val) in response.headers() {
+            if let Some(suffix) = key.as_str().strip_prefix(prefix) {
+                if let Ok(val_str) = val.to_str() {


I wonder if we should error here?

tustvold

Just some minor nits, nice to see the benefits of all the time spent DRYing up the client logic in making this a small change 😄

criccomini · 2024-06-26T23:34:46Z

@tustvold I'm ready for another review here. I think I addressed all of your comments.

I had some issues with the attributes.insert() logic after I made the change. I ended up having to do .to_string().into() in order to disentangle the lifetimes (I think?). LMK if that's not the right approach here.

criccomini · 2024-07-02T00:22:45Z

Gentle nudge here =)

tustvold

Looks good to me thank, sorry for the delay

criccomini added 4 commits June 17, 2024 17:46

Add metadata attribute

33ca8a6

Add user-defined metadata for AWS/GCP/ABS with_attributes

e75e486

Reads and writes both implemented

7825d61

Add tests for GetClient

fc17928

github-actions bot added the object-store Object Store Interface label Jun 18, 2024

Fix an indentation

7d39dfb

criccomini mentioned this pull request Jun 18, 2024

Support User-Defined Object Metadata #4754

Closed

Placate clippy

f7bb58b

Xuanwo reviewed Jun 19, 2024

View reviewed changes

Use strip_prefix and mutable attributes

da24f88

criccomini force-pushed the add-user-defined-metadata branch from ba2a828 to da24f88 Compare June 19, 2024 17:43

alamb requested a review from tustvold June 19, 2024 21:06

alamb reviewed Jun 19, 2024

View reviewed changes

tustvold reviewed Jun 23, 2024

View reviewed changes

Use static Cow for attribute metadata

8d19c44

criccomini added 2 commits June 26, 2024 16:42

Add error for value decode failure

6b1030e

Remove unnecessary into

8f9d04f

tustvold approved these changes Jul 2, 2024

View reviewed changes

tustvold merged commit cad5735 into apache:master Jul 2, 2024
13 checks passed

criccomini deleted the add-user-defined-metadata branch July 2, 2024 17:26

tveldhui mentioned this pull request Jul 10, 2024

object-store: support for client-side encryption on S3 #4753

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user defined metadata #5915

Add user defined metadata #5915

criccomini commented Jun 18, 2024

criccomini commented Jun 18, 2024

Xuanwo left a comment

Xuanwo Jun 19, 2024

criccomini Jun 19, 2024

Xuanwo Jun 19, 2024

tustvold Jun 23, 2024

Xuanwo Jun 19, 2024

Xuanwo commented Jun 19, 2024

criccomini commented Jun 19, 2024 •

edited

Loading

alamb left a comment

criccomini commented Jun 19, 2024

tustvold Jun 23, 2024

tustvold Jun 23, 2024

tustvold left a comment •

edited

Loading

criccomini commented Jun 26, 2024

criccomini commented Jul 2, 2024

tustvold left a comment

Add user defined metadata #5915

Add user defined metadata #5915

Conversation

criccomini commented Jun 18, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

criccomini commented Jun 18, 2024

Xuanwo left a comment

Choose a reason for hiding this comment

Xuanwo Jun 19, 2024

Choose a reason for hiding this comment

criccomini Jun 19, 2024

Choose a reason for hiding this comment

Xuanwo Jun 19, 2024

Choose a reason for hiding this comment

tustvold Jun 23, 2024

Choose a reason for hiding this comment

Xuanwo Jun 19, 2024

Choose a reason for hiding this comment

Xuanwo commented Jun 19, 2024

criccomini commented Jun 19, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

criccomini commented Jun 19, 2024

tustvold Jun 23, 2024

Choose a reason for hiding this comment

tustvold Jun 23, 2024

Choose a reason for hiding this comment

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

criccomini commented Jun 26, 2024

criccomini commented Jul 2, 2024

tustvold left a comment

Choose a reason for hiding this comment

criccomini commented Jun 19, 2024 •

edited

Loading

tustvold left a comment •

edited

Loading